-
Notifications
You must be signed in to change notification settings - Fork 35
first steps for kms key-ring resource and datasource #897
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it. |
panic("implement me") | ||
} | ||
|
||
func toCreatePayload(model *Model) (*kms.CreateKeyRingPayload, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider implementing a unit test for this func.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added unit test
}, nil | ||
} | ||
|
||
func mapFields(keyRing *kms.KeyRing, model *Model) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added unit test
return &keyRingResource{} | ||
} | ||
|
||
type keyRingResource struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know if you are aware of this, please don't forget to implement the import.
You can ensure this by adding the code below:
// Ensure the implementation satisfies the expected interfaces.
var (
_ resource.Resource = &keyRingResource{}
_ resource.ResourceWithConfigure = &keyRingResource{}
_ resource.ResourceWithImportState = &keyRingResource{}
)
See the git instance resource how to do it:
terraform-provider-stackit/stackit/internal/services/git/instance/resource.go
Lines 32 to 37 in 8e77675
// Ensure the implementation satisfies the expected interfaces. | |
var ( | |
_ resource.Resource = &gitResource{} | |
_ resource.ResourceWithConfigure = &gitResource{} | |
_ resource.ResourceWithImportState = &gitResource{} | |
) |
Consider doing the same for the datasource, see the git instance datasource for example:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
|
||
func (k *keyRingResource) Metadata(ctx context.Context, request resource.MetadataRequest, response *resource.MetadataResponse) { | ||
response.TypeName = request.ProviderTypeName + "kms_key_ring" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response.TypeName = request.ProviderTypeName + "kms_key_ring" | |
response.TypeName = request.ProviderTypeName + "_kms_key_ring" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
KeyRingId types.String `tfsdk:"key_ring_id"` | ||
Id types.String `tfsdk:"id"` // needed by TF | ||
ProjectId types.String `tfsdk:"project_id"` | ||
RegionId types.String `tfsdk:"region_id"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RegionId types.String `tfsdk:"region_id"` | |
Region types.String `tfsdk:"region"` # small adjustment to stick with the naming conventions across the codebase |
According to https://docs.api.stackit.cloud/documentation/kms/version/v1beta , the KMS API already uses the new multi-region concept.
See e.g. the stackit_routing_table
resource how to implement the multi-region concept. The resource has a region
field which is marked as optional and will use the default_region
configured in the provider as a fallback: https://registry.terraform.io/providers/stackitcloud/stackit/latest/docs/resources/routing_table
- You will need to store the provider configuration within your resource struct:
terraform-provider-stackit/stackit/internal/services/iaasalpha/routingtable/table/resource.go
Line 63 in 8e77675
providerData core.ProviderData terraform-provider-stackit/stackit/internal/services/iaasalpha/routingtable/table/resource.go
Lines 73 to 77 in 8e77675
var ok bool r.providerData, ok = conversion.ParseProviderData(ctx, req.ProviderData, &resp.Diagnostics) if !ok { return } - Field definition:
terraform-provider-stackit/stackit/internal/services/iaasalpha/routingtable/table/resource.go
Lines 161 to 169 in 8e77675
"region": schema.StringAttribute{ Optional: true, // must be computed to allow for storing the override value from the provider Computed: true, Description: "The resource region. If not defined, the provider region is used.", PlanModifiers: []planmodifier.String{ stringplanmodifier.RequiresReplace(), }, }, - Then use this helper func to read the region:
terraform-provider-stackit/stackit/internal/services/iaasalpha/routingtable/table/resource.go
Line 203 in 8e77675
region := r.providerData.GetRegionWithOverride(model.Region)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated region logic
if providerData.KMSCustomEndpoint != "" { | ||
apiClientConfigOptions = append(apiClientConfigOptions, config.WithEndpoint(providerData.KMSCustomEndpoint)) | ||
} else { | ||
apiClientConfigOptions = append(apiClientConfigOptions, config.WithRegion(providerData.GetRegion())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apiClientConfigOptions = append(apiClientConfigOptions, config.WithRegion(providerData.GetRegion())) | |
apiClientConfigOptions = append(apiClientConfigOptions)) |
Since the KMS API already implemented the new multi-region concept, you don't need to set the region here (or better: the SDK should throw an error if you do 😄 ).
Apart from that, consider adding a unit tests for this func, see e.g. https://github.com/stackitcloud/terraform-provider-stackit/blob/8e776757ea2280d1222afe50c3024945b4d99eed/stackit/internal/services/git/utils/util_test.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it. |
…ation # Conflicts: # go.mod # go.sum
This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it. |
@fsandel fyi |
This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it. |
This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it. |
This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it. |
This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it. |
@fsandel Could you please resolve the conflicts? |
# stackit_kms_key (Data Source) | ||
|
||
KMS Key resource schema. Must have a `region` specified in the provider configuration. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all three data sources the examples are missing. Please add them in examples/data-sources/<data-source-name>/data-source.tf like you did it for the resources
algorithm = "example algorithm" | ||
backend = "software" | ||
description = "new descr" | ||
display_name = "example name" | ||
import_only = false | ||
key_ring_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | ||
project_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | ||
purpose = "example purpose" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually have the project_id
and depending ids, like in this case key_ring_id
at the top of the example. And for the basic example we only set the required fields.
algorithm = "example algorithm" | |
backend = "software" | |
description = "new descr" | |
display_name = "example name" | |
import_only = false | |
key_ring_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | |
project_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | |
purpose = "example purpose" | |
project_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | |
key_ring_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | |
algorithm = "example algorithm" | |
backend = "software" | |
description = "example description" | |
display_name = "example name" | |
purpose = "example purpose" |
If you want you can also add an additional example, where the stackit_kms_key
resource is used together with stackit_kms_key_ring
, but this is not necessary. If you add it, please add a comment to indicate, that it's a different example
- `algorithm` (String) The encryption algorithm that the key will use to encrypt data | ||
- `backend` (String) The backend that is used for KMS. Right now, only software is accepted. | ||
- `display_name` (String) The display name to distinguish multiple keys | ||
- `import_only` (Boolean) Terraform's internal resource ID. It is structured as "`project_id`,`instance_id`". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description of import_only
doesn't fit here. And based on the api spec, it's only an optional field
- `purpose` (String) The purpose for which the key will be used | ||
|
||
### Optional | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the api spec access_scope
is missing
### Required | ||
|
||
- `algorithm` (String) The encryption algorithm that the key will use to encrypt data | ||
- `backend` (String) The backend that is used for KMS. Right now, only software is accepted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with the migration from v1beta -> v1, this was renamed to protection
key_ring_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | ||
project_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | ||
purpose = "example purpose" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an example how the resource can be imported, like we have it here
terraform-provider-stackit/docs/resources/ske_cluster.md
Lines 41 to 45 in 813b8c0
# Only use the import statement, if you want to import an existing ske cluster | |
import { | |
to = stackit_ske_cluster.import-example | |
id = "${var.project_id},${var.region},${var.ske_name}" | |
} |
description = "example description" | ||
display_name = "example name" | ||
project_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | ||
region_id = "eu01" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change also the order here, so that project_id is first and remove region_id. Also add an import example
description = "example description" | |
display_name = "example name" | |
project_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | |
region_id = "eu01" | |
} | |
project_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | |
description = "example description" | |
display_name = "example name" | |
} |
algorithm = "example algorithm" | ||
backend = "software" | ||
description = "new descr" | ||
display_name = "example name" | ||
key_ring_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | ||
project_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | ||
purpose = "example purpose" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the order, so that project_id and key_ring_id are first. And add an import example
algorithm = "example algorithm" | |
backend = "software" | |
description = "new descr" | |
display_name = "example name" | |
key_ring_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | |
project_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | |
purpose = "example purpose" | |
project_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | |
key_ring_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | |
algorithm = "example algorithm" | |
backend = "software" | |
description = "new descr" | |
display_name = "example name" | |
purpose = "example purpose" |
github.com/stackitcloud/stackit-sdk-go/services/git v0.7.1 | ||
github.com/stackitcloud/stackit-sdk-go/services/iaas v0.28.0 | ||
github.com/stackitcloud/stackit-sdk-go/services/iaasalpha v0.1.21-alpha | ||
github.com/stackitcloud/stackit-sdk-go/services/kms v0.5.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably our fault that this is quite old, but this can be update to v1.0.0. You should then get the change for backend
-> protection
and the new field access_scope
.
Please try to avoid upgrading other modules than the one that are required for you PR. I think most of them should be already in our main branch, so a rebase would probably fix this :)
) | ||
|
||
var ( | ||
_ datasource.DataSource = &keyRingDataSource{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a nitpick and most of our datasources are also missing this. But this ensures that the Configure method is implemented correct
_ datasource.DataSource = &keyRingDataSource{} | |
_ datasource.DataSource = &keyRingDataSource{} | |
_ datasource.DataSourceWithConfigure = &instanceDataSource{} |
Config: fmt.Sprintf("%s\n%s", testutil.KMSProviderConfig(), resourceKeyRingMinConfig), | ||
Check: resource.ComposeAggregateTestCheckFunc( | ||
resource.TestCheckResourceAttr("data.stackit_kms_key_ring.key_ring", "project_id", testutil.ConvertConfigVariable(testConfigKeyRingVarsMin["project_id"])), | ||
resource.TestCheckResourceAttrPair( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can also check the region
and project_id
this way btw (even if you don't set them manually in the Min
test and use the fallback values from the provider config instead) to make sure the datasource and the resource return the same values
var ( | ||
_ resource.Resource = &wrappingKeyResource{} | ||
_ resource.ResourceWithConfigure = &wrappingKeyResource{} | ||
_ resource.ResourceWithImportState = &wrappingKeyResource{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add resource.ResourceWithModifyPlan
here.
terraform-provider-stackit/stackit/internal/services/postgresflex/user/resource.go
Line 37 in 16446d7
_ resource.ResourceWithModifyPlan = &userResource{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then make sure to implement ModifyPlan
properly so the region
field works properly in case the default_region
in the provider configuration is updated.
terraform-provider-stackit/stackit/internal/services/postgresflex/user/resource.go
Lines 65 to 93 in 16446d7
// ModifyPlan implements resource.ResourceWithModifyPlan. | |
// Use the modifier to set the effective region in the current plan. | |
func (r *userResource) ModifyPlan(ctx context.Context, req resource.ModifyPlanRequest, resp *resource.ModifyPlanResponse) { // nolint:gocritic // function signature required by Terraform | |
var configModel Model | |
// skip initial empty configuration to avoid follow-up errors | |
if req.Config.Raw.IsNull() { | |
return | |
} | |
resp.Diagnostics.Append(req.Config.Get(ctx, &configModel)...) | |
if resp.Diagnostics.HasError() { | |
return | |
} | |
var planModel Model | |
resp.Diagnostics.Append(req.Plan.Get(ctx, &planModel)...) | |
if resp.Diagnostics.HasError() { | |
return | |
} | |
utils.AdaptRegion(ctx, configModel.Region, &planModel.Region, r.providerData.GetRegion(), resp) | |
if resp.Diagnostics.HasError() { | |
return | |
} | |
resp.Diagnostics.Append(resp.Plan.Set(ctx, planModel)...) | |
if resp.Diagnostics.HasError() { | |
return | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also make sure to do this for all of your resources within this PR 😊
This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it. |
Description
relates to #1234
Checklist
make fmt
examples/
directory)make generate-docs
(will be checked by CI)make test
(will be checked by CI)make lint
(will be checked by CI)